-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OAuth Integration #891
OAuth Integration #891
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits then LGTM. also, can we test outlook before merging this?
good stuff @alderwhiteford the gopher
} | ||
|
||
// Extract the user making the call: | ||
userID := c.Locals("userID").(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use a private type as the key to avoid any potential collisions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, isn't this stored in the custom claims as the issuer? it is set here when logging in
} | ||
|
||
// Create a CSRF Token: | ||
csrfToken, err := auth.GenerateURLSafeToken(32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this length a constant
config := client.ResourceClient.GetConfig() | ||
tokenUrl := fmt.Sprintf("%s/token", config.TokenURL) | ||
|
||
fmt.Println(tokenUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: debug print
if err != nil { | ||
return nil, err | ||
} | ||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can help clean this up with some new helpers in backend/utilities/http.go
package base | ||
|
||
import ( | ||
"encoding/json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fast json marshalling/unmarshalling library we are using instead
"encoding/json" | |
go_json "github.com/goccy/go-json" |
|
||
func DeleteOAuthToken(db *gorm.DB, userID uuid.UUID, resourceType models.OAuthResource) error { | ||
if err := db.Where("user_id = ? AND resource_type = ?", userID, resourceType).Delete(&models.UserOAuthTokens{}).Error; err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle not found
backend/integrations/oauth/google.go
Outdated
} | ||
|
||
func (client *GoogleOAuthClient) AuthorizeURL(state string) string { | ||
return (client.OAuthConfig.BaseURL + "/auth?" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is an example of using the stdlib's url type to build -> https://noahkreiger.medium.com/golang-building-dynamic-urls-4e93cab86e72
return client.OAuthConfig | ||
} | ||
|
||
func (client *GoogleOAuthClient) Revoke(token string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, i can help clean this up with more http utilities
backend/integrations/oauth/google.go
Outdated
func (client *GoogleOAuthClient) Revoke(token string) error { | ||
revokeUrl := fmt.Sprintf("%s/revoke?token=%s", client.OAuthConfig.TokenURL, token) | ||
|
||
req, err := http.NewRequest("POST", revokeUrl, strings.NewReader("")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be nil
instead of strings.NewReader("")
?
OAuthConfig config.OAuthSettings | ||
} | ||
|
||
func (client *OutlookOAuthClient) AuthorizeURL(state string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same idea as google
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @alderwhiteford ! 👨🍳 🔥 created the issues you mentioned in the description
Fully functioning for Google. Need to test revocation for Outlook then it will be good to go. Also need to update string concatenation - mentioned by Garrett in last PR review
Next Steps: